packages/ui/src/components/organisms/Form/DynamicForm/_stories/FileUploadShowcase/FileUploadShowcase.tsx (1)

Line range hint 95-97: Add error handling to onSubmit.

The onSubmit callback should include error handling.

           onSubmit={() => {
-            console.log('onSubmit');
+            try {
+              console.log('onSubmit');
+              // Handle form submission
+            } catch (error) {
+              console.error('Form submission failed:', error);
+              // Show error to user
+            }
           }}

packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/FileField.tsx (1)

Breaking Change Confirmed: Requires Version Bump and Migration Guide

The change to make uploadSettings required is indeed breaking as:

• DocumentField extends from FileField's interface
• Package is published to npm (@ballerine/ui@0.5.72)
• Tests verify undefined uploadSettings case

Recommendations:

• Bump major version (0.6.0) since package is pre-1.0
• Add migration guide in documentation
• Consider maintaining backward compatibility through a deprecated optional pattern

🔗 Analysis chain

Line range hint 20-26: Breaking Change: uploadSettings is now required.

Making uploadSettings required is a breaking change that could affect existing implementations. Consider:

1. Maintaining backward compatibility
2. Adding migration guide
3. Incrementing major version

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing usage of FileField to assess impact
rg -A 5 "uploadSettings[?]?:" .

Length of output: 10205

---

Script:

#!/bin/bash
# Search for FileField usage in actual implementations
rg "FileField" --type ts --type tsx -B 2 -A 2 . | grep -v "test\|story\|showcase"

# Check for any existing migration guides or documentation
fd -e md . | xargs rg -l "FileField|migration|breaking.change" 

Length of output: 174

---

Script:

#!/bin/bash
# Find all ts/tsx files and search for FileField
fd -e ts -e tsx . | xargs rg "FileField" -B 2 -A 2 | grep -v "test\|story\|showcase"

# Check package.json for version and publication status
cat packages/ui/package.json

# Look for documentation specifically in the ui package
fd -e md . packages/ui/ | xargs rg -l "FileField|migration|breaking.change"

Length of output: 15228

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/EntityFields.tsx (4)

3-3: Consider renaming the type TDeepthLevelStack.
It appears there may be a minor spelling misalignment in the type name ("Deepth" vs. "Depth"). Renaming it to something like TDepthLevelStack will improve clarity.

---

44-48: Remove or clarify the commented-out code.
Lines 44–48 contain commented-out references to useHttp. If they’re no longer necessary, removing them will keep your codebase lean. Otherwise, add a brief explanation indicating why they remain commented out.

---

67-67: Fix spelling for "documentFieldDefinitons".
Minor improvement for readability:

- const documentFieldDefinitons =
+ const documentFieldDefinitions =

---

153-194: Refine the variable name for child elements.
Using childrens can be slightly confusing. Consider a more descriptive alternative like renderableChildren or lockedChildren to clearly convey its purpose.

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/types/index.ts (1)

1-6: Double-underscore properties are slightly unconventional.
Properties like __id, __isLocked, and __isCreated work, but they may be less intuitive for some team members. Consider a naming approach that conveys the meaning without underscores if you don’t have a specific convention enforcing them.

packages/ui/src/common/hooks/useHttp/types.ts (1)

1-6: Consider enhancing the HTTP parameters interface.

1. The purpose of the required resultPath property is unclear. Consider adding JSDoc comments to explain its usage.
2. Consider adding common HTTP parameters:
   • params for query parameters
   • data or body for request payload
   • timeout for request timeout
   • withCredentials for CORS

Apply this diff to enhance the interface:

+/**
+ * Parameters for making HTTP requests
+ */
 export interface IHttpParams {
   url: string;
+  /**
+   * @description Path to extract the result from the response
+   */
   resultPath: string;
   headers?: Record<string, string>;
   method?: 'POST' | 'PUT' | 'GET' | 'DELETE';
+  params?: Record<string, string>;
+  data?: unknown;
+  timeout?: number;
+  withCredentials?: boolean;
 }

packages/ui/src/common/hooks/useHttp/utils/format-headers.ts (1)

3-15: Add validation and error handling for header formatting.

The current implementation lacks validation and error handling for header names and values.

Apply this diff to enhance the function:

+const isValidHeaderName = (name: string) => /^[a-zA-Z0-9-]+$/.test(name);
+const isValidHeaderValue = (value: string) => !value.includes('\n') && !value.includes('\r');
+
 export const formatHeaders = (
   headers: Record<string, string>,
   metadata: Record<string, string> = {},
 ) => {
   const formattedHeaders: Record<string, string> = {};
 
   Object.entries(headers).forEach(([key, value]) => {
+    if (!isValidHeaderName(key)) {
+      throw new Error(`Invalid header name: ${key}`);
+    }
+    if (!isValidHeaderValue(value)) {
+      throw new Error(`Invalid header value for ${key}`);
+    }
     const formattedValue = formatString(value, metadata);
+    if (!isValidHeaderValue(formattedValue)) {
+      throw new Error(`Invalid formatted header value for ${key}`);
+    }
     formattedHeaders[key] = formattedValue;
   });
 
   return formattedHeaders;
 };

packages/ui/src/common/hooks/useHttp/utils/request.ts (1)

24-28: Improve error handling.

The current error handling could be enhanced:

1. Avoid logging raw errors as they might contain sensitive information.
2. Consider transforming the error before re-throwing to ensure consistent error format.

   } catch (error) {
-    console.error('Failed to perform request.', error);
+    console.error('Failed to perform request.');
+    
+    // Transform error to ensure consistent format
+    if (axios.isAxiosError(error)) {
+      throw {
+        message: error.response?.data?.message || 'Request failed',
+        status: error.response?.status,
+        code: error.code
+      };
+    }

     throw error;
   }

packages/ui/src/common/hooks/useHttp/useHttp.ts (2)

20-22: Improve error type handling.

The current error type casting to Error might be unsafe. Consider using a more specific error type or proper type checking.

-        setResponseError(error as Error);
+        if (error instanceof Error) {
+          setResponseError(error);
+        } else {
+          setResponseError(new Error('An unknown error occurred'));
+        }

---

11-27: Add type information for response data.

Consider adding generic type parameters to improve type safety for the response data.

-  const runRequest = useCallback(
+  const runRequest = useCallback<(requestPayload?: any) => Promise<T | undefined>>(
     async (requestPayload?: any) => {

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/hooks/useChildrenDisabledOnLock/useChildrenDisabledOnLock.ts (1)

16-24: Replace hardcoded disable rule with a more meaningful condition.

The current disable rule {==: [1, 1]} is hardcoded to always be true. Consider using a more meaningful condition that reflects the actual locking state.

           disable: [
             ...(child.disable || []),
             {
               engine: 'json-logic' as const,
               value: {
-                '==': [1, 1],
+                var: 'isLocked'
               },
             },
           ],

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/hooks/useEntityLock/useEntityLock.ts (2)

22-22: Fix typo in comment.

The comment has a typo: "it it" should be "it".

-    // If the entity has an id, it it means its already created and should't be mutated.
+    // If the entity has an id, it means it's already created and shouldn't be mutated.

---

41-58: Optimize performance by avoiding the delete operator.

Using the delete operator can impact performance. Consider using an undefined assignment instead.

 const unlockEntity = useCallback(() => {
   const updatedArray = entities.map(entity => {
     if (entity.__id === entityId) {
       const newEntity = {
         ...entity,
       };

-      delete newEntity.__isLocked;
+      newEntity.__isLocked = undefined;
       onUnlock(newEntity);

       return newEntity;
     }

     return entity;
   });

   onChange(updatedArray);
 }, [onChange, entityId, entities, onUnlock]);

🧰 Tools

🪛 Biome (1.9.4)

[error] 48-48: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/hooks/useEntityFieldGroupList/useEntityFieldGroupList.ts (2)

51-57: Improve error handling.

The catch block should:

1. Use a more specific error type
2. Include error details in the toast message
3. Consider retrying the operation

-        } catch (error) {
-          toast.error('Failed to delete entity.');
+        } catch (error: unknown) {
+          const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+          toast.error(`Failed to delete entity: ${errorMessage}`);

---

36-38: Add type guard for value array.

The value array access needs better type safety.

-      if (!Array.isArray(value)) {
+      if (!Array.isArray(value) || !value.length) {
         return;
       }

packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/hooks/useFileUpload/useFileUpload.ts (1)

42-44: Refactor duplicate FormData creation.

The FormData creation logic is duplicated between 'change' and 'submit' modes.

Extract to a helper function:

+  const createFileFormData = useCallback((file: File) => {
+    const formData = new FormData();
+    formData.append('file', file);
+    return formData;
+  }, []);

-          const formData = new FormData();
-          formData.append('file', e.target?.files?.[0] as File);
+          const formData = createFileFormData(e.target?.files?.[0] as File);

Also applies to: 57-58

packages/ui/src/components/organisms/Form/DynamicForm/_stories/FileUploadShowcase/FileUploadShowcase.tsx (1)

17-19: Make upload URLs configurable.

The localhost URLs should be configurable to support different environments.

+const API_BASE_URL = process.env.REACT_APP_API_URL || 'http://localhost:3000';
+
 const schema: Array<IFormElement<any, any>> = [
   {
     id: 'FileField:Regular',
     element: 'filefield',
     valueDestination: 'file-regular',
     params: {
       label: 'Regular Upload',
       placeholder: 'Select File',
       uploadSettings: {
-        url: 'http://localhost:3000/upload',
+        url: `${API_BASE_URL}/upload`,
         resultPath: 'filename',
         method: 'POST',
       },
     },
   },

Also applies to: 31-35, 49-51, 68-70

packages/ui/src/components/organisms/Form/Validator/validators/document/document-validator.unit.test.ts (2)

17-21: Add more specific error messages.

The error messages in the tests should be more descriptive to help identify the failure cause.

-      'Test message',
+      'Value must be an array of documents',
     );
   });

   it('should throw error when array is empty', () => {
     expect(() => documentValidator([], mockParams)).toThrow(
-      'Test message',
+      'Document array cannot be empty',
     );
   });

Also applies to: 23-25

---

51-61: Add edge case tests.

The test suite should include edge cases such as:

• Documents with multiple pages
• Invalid page numbers
• Special characters in IDs

Would you like me to generate additional test cases for these scenarios?

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.tsx (2)

20-26: Consider enhancing type safety for HTTP parameters.

The httpsParams property could benefit from more specific typing for request methods and URL patterns.

 export interface IEntityFieldGroupParams extends IFieldListParams {
   httpsParams: {
-    createEntity: IHttpParams;
-    deleteEntity: IHttpParams;
+    createEntity: IHttpParams & { method: 'POST' | 'PUT' };
+    deleteEntity: IHttpParams & { method: 'DELETE' };
   };
   lockText?: string;
 }

---

37-37: Consider memoizing callback functions.

The addItem and removeItem callbacks from useEntityFieldGroupList should be memoized to prevent unnecessary re-renders.

-  const { items, isRemovingEntity, addItem, removeItem } = useEntityFieldGroupList({ element });
+  const { items, isRemovingEntity, addItem: rawAddItem, removeItem: rawRemoveItem } = useEntityFieldGroupList({ element });
+  const addItem = useCallback(rawAddItem, [rawAddItem]);
+  const removeItem = useCallback(rawRemoveItem, [rawRemoveItem]);

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/hooks/useChildrenDisabledOnLock/useChildrenDisabledOnLock.unit.test.ts (1)

41-46: Consider using a constant for the disable rule.

The disable rule is duplicated across test cases and could be extracted to a constant.

+const LOCK_DISABLE_RULE = {
+  engine: 'json-logic',
+  value: {
+    '==': [1, 1],
+  },
+};

-    const expectedDisableRule = {
-      engine: 'json-logic',
-      value: {
-        '==': [1, 1],
-      },
-    };
+    const expectedDisableRule = LOCK_DISABLE_RULE;

packages/ui/src/common/hooks/useHttp/utils/request.unit.test.ts (1)

75-91: Add more specific error test cases.

The error handling test could be expanded to cover specific HTTP error codes and network errors.

+  it('should handle network errors', async () => {
+    const requestParams = {
+      url: 'http://api.example.com/test',
+      method: 'GET',
+      headers: {},
+    } as const;
+    const error = new Error('Network Error');
+    error.name = 'NetworkError';
+
+    mockFormatString.mockReturnValue('http://api.example.com/test');
+    mockFormatHeaders.mockReturnValue({});
+    mockAxios.mockRejectedValue(error);
+
+    const consoleSpy = vi.spyOn(console, 'error');
+
+    await expect(request(requestParams, {})).rejects.toThrow('Network Error');
+    expect(consoleSpy).toHaveBeenCalledWith('Failed to perform request.', error);
+  });

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.stories.tsx (1)

116-122: Enhance story documentation and add variants.

The story could benefit from better documentation and additional variants to showcase different states.

 export default {
   component: EntityFieldGroup,
+  parameters: {
+    docs: {
+      description: {
+        component: 'A dynamic form field group that manages a list of entity fields.',
+      },
+    },
+  },
 };

 export const Default = {
   render: () => <EntityFieldGroup />,
+  parameters: {
+    docs: {
+      description: {
+        story: 'Default state with pre-filled data.',
+      },
+    },
+  },
 };
+
+export const Empty = {
+  render: () => <EntityFieldGroup />,
+  args: {
+    initialContext: {},
+  },
+};

packages/ui/src/common/hooks/useHttp/useHttp.unit.test.ts (3)

92-109: Consider using a shorter timeout for testing loading state.

The test uses a 100ms timeout which might slow down the test suite unnecessarily. Consider using a shorter timeout (e.g., 10ms) since we're only testing state transitions.

 vi.mocked(request).mockImplementationOnce(
   () =>
     new Promise(resolve => {
-      setTimeout(() => resolve(mockResponse), 100);
+      setTimeout(() => resolve(mockResponse), 10);
     }),
 );

---

78-90: Enhance error test coverage.

The error test could be more thorough by:

1. Testing different types of errors (network, validation, etc.)
2. Verifying error message propagation
3. Testing error state reset on subsequent successful requests

---

9-14: Consider using TypeScript for mock parameters.

The mockParams object could benefit from explicit type annotations to ensure type safety in tests.

-  const mockParams = {
+  const mockParams: IHttpParams = {
     url: 'test-url',
     resultPath: 'data.items',
     method: 'GET' as const,
     headers: {},
   };

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/hooks/useEntityFieldGroupList/useEntityFieldGroupList.unit.test.ts (2)

64-67: Avoid using 'any' type in mock implementation.

The cryptoSpy mock uses 'any' type which bypasses TypeScript's type checking. Consider using a more specific type.

-    cryptoSpy.mockReturnValue(mockUUID as any);
+    cryptoSpy.mockReturnValue(mockUUID);

---

95-107: Enhance error case test coverage.

The test for handling undefined value could be expanded to cover more edge cases:

• null values
• non-array objects
• invalid array items

packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/FileField.tsx (1)

51-57: Optimize file creation for string values.

Creating an empty File object for string values is inefficient. Consider using a separate type or interface to represent file identifiers.

-    if (typeof value === 'string') {
-      return new File([], value);
-    }
+    if (typeof value === 'string') {
+      return { id: value, name: value } as FileIdentifier;
+    }

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/EntityFieldGroupDocument.tsx (1)

67-71: Fix typo in warning message.

The warning message contains a typo: "migging" should be "missing".

-      console.warn('Template id is migging in element', element);
+      console.warn('Template id is missing in element', element);

packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/hooks/useFileUpload/useFileUpload.unit.test.ts (2)

100-114: Consider adding assertions for task properties.

While the test verifies basic task creation, it would be more robust to assert the specific properties of the added task.

   await result.current.handleChange(mockEvent);

   expect(mockOnChange).toHaveBeenCalledWith(mockFile);
-  expect(mockAddTask).toHaveBeenCalled();
+  expect(mockAddTask).toHaveBeenCalledWith(expect.objectContaining({
+    id: mockElement.id,
+    run: expect.any(Function)
+  }));

---

192-202: Consider adding more loading state scenarios.

While the basic loading state is tested, consider adding tests for:

1. State transitions (idle → loading → complete)
2. Multiple concurrent uploads
3. Loading state during error scenarios

packages/ui/src/components/organisms/Form/Validator/validators/document/document-validator.ts (1)

5-30: 🛠️ Refactor suggestion

Enhance error handling and validation in document validator.

The validator uses the same error message for different validation failures and lacks bounds checking.

Apply this diff to improve the validator:

 export const documentValidator: TValidator<
   TDocument[],
   IDocumentValidatorParams,
   TBaseValidators | 'document'
 > = (value, params) => {
-  const { message = 'Document is required' } = params;
+  const {
+    message = {
+      required: 'Document is required',
+      notFound: 'Document not found',
+      invalidPage: 'Invalid page number',
+      missingProperty: 'Required document property not found',
+    },
+  } = params;
   const { id, pageNumber = 0, pageProperty = 'ballerineFileId' } = params.value;
 
+  if (pageNumber < 0) {
+    throw new Error(message.invalidPage);
+  }
+
   if (!Array.isArray(value) || !value.length) {
-    throw new Error(message);
+    throw new Error(message.required);
   }
 
   const document = value.find(doc => doc.id === id);
 
   if (!document) {
-    throw new Error(message);
+    throw new Error(message.notFound);
   }
 
+  if (pageNumber >= document.pages.length) {
+    throw new Error(message.invalidPage);
+  }
+
   const documentValue = document.pages[pageNumber]?.[pageProperty];
 
   if (!documentValue) {
-    throw new Error(message);
+    throw new Error(message.missingProperty);
   }
 
   return true;
 };

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

export const documentValidator: TValidator<
  TDocument[],
  IDocumentValidatorParams,
  TBaseValidators | 'document'
> = (value, params) => {
  const {
    message = {
      required: 'Document is required',
      notFound: 'Document not found',
      invalidPage: 'Invalid page number',
      missingProperty: 'Required document property not found',
    },
  } = params;
  const { id, pageNumber = 0, pageProperty = 'ballerineFileId' } = params.value;

  if (pageNumber < 0) {
    throw new Error(message.invalidPage);
  }

  if (!Array.isArray(value) || !value.length) {
    throw new Error(message.required);
  }

  const document = value.find(doc => doc.id === id);

  if (!document) {
    throw new Error(message.notFound);
  }

  if (pageNumber >= document.pages.length) {
    throw new Error(message.invalidPage);
  }

  const documentValue = document.pages[pageNumber]?.[pageProperty];

  if (!documentValue) {
    throw new Error(message.missingProperty);
  }

  return true;
};

packages/ui/src/common/hooks/useHttp/utils/request.ts (1)

7-7: ⚠️ Potential issue

Fix typo in type name.

The type name TReuqestParams contains a typo and should be TRequestParams.

-export type TReuqestParams = Omit<IHttpParams, 'resultPath'>;
+export type TRequestParams = Omit<IHttpParams, 'resultPath'>;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

export type TRequestParams = Omit<IHttpParams, 'resultPath'>;

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/hooks/useEntityFieldGroupList/useEntityFieldGroupList.ts (1)

42-43: ⚠️ Potential issue

Remove hardcoded condition.

The if (true) condition is replacing actual entity existence check logic. This could lead to unnecessary API calls and incorrect behavior.

-      // if (entity?.id) {
-      if (true) {
+      if (entity?.id) {

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

      if (entity?.id) {

🧰 Tools

🪛 Biome (1.9.4)

[error] 43-43: Unexpected constant condition.

(lint/correctness/noConstantCondition)

packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/hooks/useFileUpload/useFileUpload.ts (1)

41-47: 🛠️ Refactor suggestion

Add file validation.

The hook should validate file existence and potentially file type before upload.

+      const file = e.target?.files?.[0];
+      if (!file) {
+        console.error('No file selected');
+        return;
+      }
+      
+      if (params.allowedTypes && !params.allowedTypes.includes(file.type)) {
+        console.error('Invalid file type');
+        return;
+      }

       if (uploadOn === 'change') {
         try {
-          const formData = new FormData();
-          formData.append('file', e.target?.files?.[0] as File);
+          const formData = createFileFormData(file);

Also applies to: 56-61

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.tsx (1)

57-57: 🛠️ Refactor suggestion

Replace non-null assertions with safe alternatives.

Using non-null assertions (!) can lead to runtime errors if the assumptions are wrong.

-            entityId={entity.__id!}
+            entityId={entity.__id ?? throw new Error('Entity ID is required')}
-            onRemoveClick={() => removeItem(entity.__id!)}
+            onRemoveClick={() => entity.__id && removeItem(entity.__id)}

Also applies to: 61-61

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/hooks/useEntityFieldGroupList/useEntityFieldGroupList.unit.test.ts (1)

57-74: 🛠️ Refactor suggestion

Use try-finally to ensure spy restoration.

The cryptoSpy should be restored even if the test fails. Wrap the test in a try-finally block.

   it('should add new item with generated __id', async () => {
     const mockOnChange = vi.fn();
     vi.mocked(useField).mockReturnValue({
       onChange: mockOnChange,
       value: [],
     } as unknown as ReturnType<typeof useField>);

     const mockUUID = '123-456';
     const cryptoSpy = vi.spyOn(crypto, 'randomUUID');
     cryptoSpy.mockReturnValue(mockUUID);
+    try {
       const { result } = renderHook(() => useEntityFieldGroupList({ element: mockElement }));
       await result.current.addItem();
       expect(mockOnChange).toHaveBeenCalledWith([{ __id: mockUUID }]);
+    } finally {
       cryptoSpy.mockRestore();
+    }
   });

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

  it('should add new item with generated __id', async () => {
    const mockOnChange = vi.fn();
    vi.mocked(useField).mockReturnValue({
      onChange: mockOnChange,
      value: [],
    } as unknown as ReturnType<typeof useField>);

    const mockUUID = '123-456';
    const cryptoSpy = vi.spyOn(crypto, 'randomUUID');
    cryptoSpy.mockReturnValue(mockUUID);
    try {
      const { result } = renderHook(() => useEntityFieldGroupList({ element: mockElement }));
      await result.current.addItem();
      expect(mockOnChange).toHaveBeenCalledWith([{ __id: mockUUID }]);
    } finally {
      cryptoSpy.mockRestore();
    }
  });

packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/FileField.tsx (1)

35-37: ⚠️ Potential issue

Replace non-null assertion with runtime check.

Using non-null assertion (!) is risky as it bypasses TypeScript's null checks. Consider adding a runtime check instead.

-  const { handleChange, isUploading: disabledWhileUploading } = useFileUpload(
-    element,
-    element.params!,
-  );
+  if (!element.params) {
+    throw new Error('FileField requires params to be defined');
+  }
+  const { handleChange, isUploading: disabledWhileUploading } = useFileUpload(
+    element,
+    element.params,
+  );

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    if (!element.params) {
      throw new Error('FileField requires params to be defined');
    }
    const { handleChange, isUploading: disabledWhileUploading } = useFileUpload(
      element,
      element.params,
    );

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/EntityFieldGroupDocument.tsx (2)

73-76: ⚠️ Potential issue

Avoid type assertion for template ID.

Using type assertion for the template ID is risky. Consider adding runtime type checking.

+    const templateId = element.params?.template?.id;
+    if (typeof templateId !== 'string') {
+      console.warn('Invalid template ID type', templateId);
+      return;
+    }
     const updatedDocuments = removeDocumentFromListByTemplateId(
       documentsList,
-      element.params?.template?.id as string,
+      templateId,
     );

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    const templateId = element.params?.template?.id;
    if (typeof templateId !== 'string') {
      console.warn('Invalid template ID type', templateId);
      return;
    }
    const updatedDocuments = removeDocumentFromListByTemplateId(
      documentsList,
      templateId,
    );

---

85-96: ⚠️ Potential issue

Add type safety for file handling.

The file type assertion could lead to runtime errors. Consider adding validation and proper error handling.

     (e: React.ChangeEvent<HTMLInputElement>) => {
       const documents = get(documentsList || [], element.valueDestination);
+      const file = e.target.files?.[0];
+      if (!file) {
+        console.warn('No file selected');
+        return;
+      }
       const updatedDocuments = createOrUpdateFileIdOrFileInDocuments(
         documents,
         element,
-        e.target.files?.[0] as File,
+        file,
       );
       onChange(updatedDocuments);
     },

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

  const handleChange = useCallback(
    (e: React.ChangeEvent<HTMLInputElement>) => {
      const documents = get(documentsList || [], element.valueDestination);
      const file = e.target.files?.[0];
      if (!file) {
        console.warn('No file selected');
        return;
      }
      const updatedDocuments = createOrUpdateFileIdOrFileInDocuments(
        documents,
        element,
        file,
      );
      onChange(updatedDocuments);
    },
    [onChange],
  );

packages/ui/src/components/organisms/Form/DynamicForm/fields/FileField/hooks/useFileUpload/useFileUpload.unit.test.ts (2)

140-182: 🛠️ Refactor suggestion

Standardize error handling across upload modes.

The error handling differs between immediate and submit-time uploads. Consider standardizing the approach:

1. Use error callbacks instead of console.error
2. Ensure consistent error object structure
3. Add type safety for error objects

-    expect(consoleSpy).toHaveBeenCalledWith('Failed to upload file.', expect.any(Error));
+    expect(mockOnChange).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({
+          message: 'Upload failed'
+        })
+      })
+    );

Committable suggestion skipped: line range outside the PR's diff.

---

117-138: 🛠️ Refactor suggestion

Consider enhancing error handling.

Using console.log for error reporting is not ideal for production code. Consider:

1. Throwing a specific error type
2. Using a proper error handling mechanism
3. Implementing error callbacks

-    expect(consoleSpy).toHaveBeenCalledWith('Failed to upload, no upload settings provided');
+    expect(mockOnChange).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: new Error('No upload settings provided')
+      })
+    );

Committable suggestion skipped: line range outside the PR's diff.

packages/ui/src/components/organisms/Form/Validator/validators/document/document-validator.unit.test.ts (4)

8-15: Consider improving type safety of mock data.

While the mock data structure is appropriate, consider removing the type assertion and explicitly defining the mock data with proper types.

-  const mockParams = {
+  const mockParams: ICommonValidator<IDocumentValidatorParams, TBaseValidators | 'document'> = {
     message: 'Test message',
     value: {
       id: 'test-id',
       pageNumber: 0,
       pageProperty: 'ballerineFileId',
     },
-  } as ICommonValidator<IDocumentValidatorParams, TBaseValidators | 'document'>;
+  };

---

17-31: Improve type safety in test data and add boundary tests.

The test cases for invalid input could be improved:

1. Remove type assertions and use proper typing:

-    expect(() => documentValidator(null as unknown as TDocument[], mockParams)).toThrow(
+    expect(() => documentValidator(null as any, mockParams)).toThrow(

2. Add boundary tests for array size limits:

it('should handle large document arrays', () => {
  const largeDocArray = Array(1000).fill({
    id: 'test-id',
    pages: [{ ballerineFileId: 'valid-file-id' }],
    propertiesSchema: {},
  });
  expect(documentValidator(largeDocArray, mockParams)).toBe(true);
});

---

33-49: Make test descriptions more specific.

The test descriptions could be more descriptive about the exact scenario being tested.

-  it('should throw error when document page does not exist', () => {
+  it('should throw error when document has empty pages array', () => {

-  it('should throw error when document page property does not exist', () => {
+  it('should throw error when ballerineFileId is missing from document page', () => {

---

51-80: Add test cases for edge cases and error messages.

The valid document and default parameter tests look good, but consider adding:

1. Test for exact error message content
2. Test for multiple pages in a document
3. Test for multiple documents in the array

Example:

it('should handle multiple pages in a document', () => {
  const mockDocuments = [{
    id: 'test-id',
    pages: [
      { ballerineFileId: 'file-1' },
      { ballerineFileId: 'file-2' }
    ],
    propertiesSchema: {},
  }] as TDocument[];
  
  expect(documentValidator(mockDocuments, {
    ...mockParams,
    value: { ...mockParams.value, pageNumber: 1 }
  })).toBe(true);
});

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/hooks/useEntityLock/useEntityLock.ts (3)

7-14: Consider adding TypeScript types for callback parameters.

The onLock and onUnlock callback parameters would benefit from explicit TypeScript types to ensure type safety across component boundaries.

-  onLock: (entity: IEntity) => void,
-  onUnlock: (entity: IEntity) => void,
+  onLock: (entity: Readonly<IEntity>) => void,
+  onUnlock: (entity: Readonly<IEntity>) => void,

---

21-24: Improve comment clarity.

The comment appears to have a typo and could be more descriptive about the locking behavior.

-    // If the entity has an id, it it means its already created and should't be mutated.
+    // Check if the entity is locked to prevent mutations

---

41-58: Optimize performance by avoiding object spread in map.

The unlockEntity function creates a new object for each entity in the array, even when not needed.

   const unlockEntity = useCallback(() => {
+    if (!entities?.length) return;
     const updatedArray = entities.map(entity => {
       if (entity.__id === entityId) {
-        const newEntity = {
-          ...entity,
-        };
-        delete newEntity.__isLocked;
+        const { __isLocked, ...newEntity } = entity;
         onUnlock(newEntity);
         return newEntity;
       }
       return entity;
     });

🧰 Tools

🪛 Biome (1.9.4)

[error] 48-48: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.tsx (1)

70-74: Add ARIA label for better accessibility.

The add button should have an ARIA label for better screen reader support.

-        <Button onClick={addItem} disabled={disabled}>
+        <Button 
+          onClick={addItem} 
+          disabled={disabled}
+          aria-label={`Add ${addButtonLabel}`}>

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.stories.tsx (1)

106-106: Remove commented-out code.

The commented-out onEvent prop should be removed.

-          // onEvent={console.log}

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/EntityFieldGroupDocument.tsx (3)

67-69: Fix typo in console warning message.

The warning message contains a typo in the word "missing".

-      console.warn('Template id is migging in element', element);
+      console.warn('Template id is missing in element', element);

---

112-112: Fix typo in UI text.

The text "No File Choosen" contains a spelling error.

-        <span className="truncate text-sm">{file ? file.name : 'No File Choosen'}</span>
+        <span className="truncate text-sm">{file ? file.name : 'No File Chosen'}</span>

---

98-145: Add keyboard navigation support.

The component should support keyboard navigation for better accessibility.

Add keyboard event handlers to support Enter and Space key interactions:

       <div
         className={ctw(
           'relative flex h-[56px] flex-row items-center gap-3 rounded-[16px] border bg-white px-4',
           { 'pointer-events-none opacity-50': disabled },
         )}
         onClick={focusInputOnContainerClick}
+        onKeyDown={(e) => {
+          if (e.key === 'Enter' || e.key === ' ') {
+            e.preventDefault();
+            focusInputOnContainerClick();
+          }
+        }}
+        role="button"
+        tabIndex={0}
         data-testid={createTestId(element, stack)}
       >

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFields/EntityFields.tsx (2)

90-90: Replace hardcoded delay with configuration.

The hardcoded delay should be moved to a configuration or environment variable for better flexibility.

-await delay(1000);
+await delay(config.entityCreationDelay);

---

113-118: Enhance error handling with specific error types.

The current error handling is too generic. Consider handling specific error types and providing more detailed error messages.

-} catch (error) {
-  toast.error('Failed to create entity.');
-  console.error(error);
-
-  return context;
+} catch (error) {
+  console.error('Entity creation failed:', error);
+  if (error instanceof NetworkError) {
+    toast.error('Network error while creating entity. Please try again.');
+  } else if (error instanceof ValidationError) {
+    toast.error('Invalid entity data. Please check your input.');
+  } else {
+    toast.error('Failed to create entity. Please try again later.');
+  }
+  return context;

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.tsx (1)

73-74: ⚠️ Potential issue

Fix inconsistent key usage.

The component uses entity.id as key but entity.__id for other operations. This could lead to rendering issues.

-            key={entity.id}
+            key={entity.__id!}

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/EntityFieldGroupDocument.tsx (1)

87-87: ⚠️ Potential issue

Fix typo in warning message.

The word "migging" should be "missing". Also, follow the suggested format from the past review.

-      console.warn('Template id is migging in element', element);
+      console.warn('Template id is missing in element', element);

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/providers/EntityFieldGroupTypeProvider/types.ts (1)

3-5: LGTM! Consider adding JSDoc comments.

The interface is well-defined. Consider adding JSDoc comments to document the purpose of the interface and its property.

+/**
+ * Context interface for EntityFieldGroupType provider
+ */
 export interface IEntityFieldGroupTypeContext {
+  /** The type of entity field group, if defined */
   entityFieldGroupType?: TEntityFieldGroupType;
 }

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/providers/EntityFieldGroupTypeProvider/hooks/external/useEntityFieldGroupType/useEntityFieldGroupType.ts (1)

4-12: LGTM! Consider enhancing the error message.

The hook implementation follows React best practices with proper error handling.

Consider making the error message more descriptive:

-    throw new Error('useEntityFieldGroupType must be used within a EntityFieldGroupTypeProvider');
+    throw new Error('useEntityFieldGroupType must be used within an EntityFieldGroupTypeProvider component');

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/providers/EntityFieldGroupTypeProvider/EntityFieldGroupTypeProvider.tsx (1)

5-8: LGTM! Consider adding JSDoc comments for props interface.

The props interface is well-defined.

+/**
+ * Props for EntityFieldGroupTypeProvider component
+ */
 interface IEntityFieldGroupTypeProviderProps {
+  /** Child components to be wrapped by the provider */
   children: React.ReactNode;
+  /** Optional type of the entity field group */
   entityFieldGroupType?: TEntityFieldGroupType;
 }

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/components/EntityFieldGroupDocument/helpers/get-entity-field-group-document-value-destination.ts (1)

3-14: Document the $0 placeholder usage.

The paths contain a $0 placeholder which should be documented for clarity. Also, consider making the error message consistent with getEntityGroupValueDestination.

Apply these changes:

+// The $0 placeholder in the paths represents the index of the entity in the array
 export const getEntityFieldGroupDocumentValueDestination = (type: TEntityFieldGroupType) => {
   const valueDestinationsMap: Record<TEntityFieldGroupType, string> = {
     director: 'entity.data.additionalInfo.directors[$0].additionalInfo.documents',
     ubo: 'entity.data.additionalInfo.ubos[$0].documents',
   };

   if (!valueDestinationsMap[type]) {
-    throw new Error(`Invalid entity field group type in documentfield: ${type}`);
+    throw new Error(`Invalid entity group type: ${type}`);
   }

   return valueDestinationsMap[type];
 };

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.tsx (1)

95-95: Consider managing Toaster cleanup.

The Toaster component is rendered but there's no cleanup logic. Consider wrapping it in a separate component that handles cleanup on unmount.

-        <Toaster />
+        <ToasterWithCleanup />

Create a new component:

const ToasterWithCleanup = () => {
  useEffect(() => {
    return () => {
      // Clean up toasts on unmount
      toast.dismiss();
    };
  }, []);

  return <Toaster />;
};

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.stories.tsx (1)

107-107: Remove commented out code.

Remove commented out code that is not being used.

-          // onEvent={console.log}

Also applies to: 220-220, 329-329

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

services/workflows-service/src/collection-flow/collection-flow.module.ts (1)

79-81: Remove duplicate provider declarations.

The following providers are declared twice in the providers array:

• StorageService (lines 79 and 91)
• FileRepository (lines 80 and 92)

Apply this diff to remove the duplicates:

    EntityRepository,
    StorageService,
    FileRepository,
    HookCallbackHandlerService,
    FileService,
    WorkflowEventEmitterService,
    FilterRepository,
    FilterService,
    UserService,
    UserRepository,
    PasswordService,
    CustomerService,
    CustomerRepository,
-   StorageService,
-   FileRepository,
    SalesforceService,
    SalesforceIntegrationRepository,
    SentryService,

Also applies to: 91-92

packages/ui/src/components/organisms/Form/DynamicForm/fields/EntityFieldGroup/EntityFieldGroup.stories.tsx (2)

103-105: ⚠️ Potential issue

Remove console.log statements from onSubmit handlers.

Production code should not contain console.log statements.

Apply this diff to all onSubmit handlers:

-          onSubmit={() => {
-            console.log('onSubmit');
-          }}
+          onSubmit={() => {
+            // Handle form submission
+          }}

Also applies to: 436-438, 553-555

---

12-92: 🛠️ Refactor suggestion

Reduce code duplication in schema definitions.

The schema definitions for default, ubos, and directors are nearly identical, differing only in the type parameter and some field configurations.

Follow the previous suggestion to extract common schema into a factory function.

Also applies to: 125-407, 462-542